Add edge-case tests for weighted diameter; document asymmetric-weight bug#1
Open
Krastanov-agent wants to merge 2 commits intoLoveLow-Global:optimize-diameter-2from
Open
Conversation
… bug Add 50 edge-case tests for the weighted diameter algorithm covering: - Empty/single-vertex/two-vertex graphs - Path, cycle, star, complete, grid, Petersen, wheel, barbell, caterpillar, tree graphs - Directed and undirected variants - Integer, Float32, Float64 weight matrices - Very large/small/zero weights - Disconnected graphs (should return Inf/typemax) - Hub far from diameter endpoints - Dense and sparse random graphs - Stress tests (30+ random ER seeds for each of directed/undirected) One test demonstrates a bug: _diameter_weighted_undirected returns wrong results with asymmetric weight matrices (distmx[i,j] != distmx[j,i]). The iFUB bound "lb >= 2*d_prev" assumes d(v,hub)=d(hub,v), which fails when the weight matrix is not symmetric. Failure rate ~77% on random inputs. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Two fixes: 1. _diameter_weighted_undirected: track both forward and backward distance fringes from the hub vertex, using the transposed weight matrix for backward distances. The iFUB stopping bound "lb >= 2*d_prev" requires that for any unprocessed vertex v, BOTH d(hub,v) and d(v,hub) are <= d_prev. With asymmetric weights (distmx[i,j] != distmx[j,i]), d(v,hub) can greatly exceed d(hub,v), invalidating the bound and causing the algorithm to return wrong results (~77% failure rate on random asymmetric weights). The fix mirrors the directed algorithm's approach: compute Dijkstra from the hub with both distmx and permutedims(distmx), group vertices into forward and backward fringes, and process both. For fringe_fwd vertices, compute in-eccentricity (Dijkstra with transposed weights); for fringe_bwd, compute out-eccentricity (Dijkstra with original weights). 2. _diameter_weighted_directed: change breaking condition from "lb > 2*d_prev" to "lb >= 2*d_prev" for consistency with the undirected case. The >= condition is sufficient because when lb == 2*d_prev, all remaining vertex eccentricities are bounded by 2*d_prev <= lb, so no further processing is needed. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
The suggested solution here might be stupid. Instead of doubling the number of checks, maybe early one we should be checking for asymmetry. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR adds a comprehensive edge-case test suite (
test/test_diameter_edge_cases.jl) for the weighted diameter algorithm introduced in JuliaGraphs#495. It includes 50 targeted test cases and one bug report found through systematic testing.Test results: 180 pass, 1 fail
The 1 failure is an intentional test that demonstrates a bug (see below).
Bug Found: Asymmetric Weight Matrix on Undirected Graphs
Description
_diameter_weighted_undirectedproduces wrong results when the weight matrix is not symmetric (distmx[i,j] != distmx[j,i]).Minimal reproducer
Root cause analysis
The iFUB stopping condition at
src/distance.jl:330:relies on the bound: for any unprocessed vertex
vwithd(hub, v) ≤ d_prev, the eccentricity satisfiesecc(v) ≤ 2 * d_prev. This comes from the triangle inequality:The problem: the algorithm only computes
d(hub, v)(forward Dijkstra from hub), but the bound requiresd(v, hub)(the reverse direction). For undirected graphs with symmetric weights,d(v, hub) = d(hub, v), so the bound holds. But whendistmx[i,j] ≠ distmx[j,i], the two distances diverge and the bound becomes invalid.Detailed trace of the bug
Impact
maximum(eccentricity(g, distmx))handled this correctly_diameter_weighted_directed) correctly tracks both forward and backward distances via separate fringes, so it handles asymmetric weights properlyPossible fixes
distmx = (distmx + distmx') / 2— changes semanticsdistmx ≠ distmx'— correct but slowerdistmxmust be symmetric for undirected graphs — least code changeMinor observation:
>=vs>in breaking conditiondistance.jl:330):lb >= 2 * d_prevdistance.jl:290):lb > 2 * d_prevThe directed
>is slightly more conservative (processes one extra fringe). Testing confirms both>=and>produce correct results for directed graphs (1948 additional tests), so this is just a minor inefficiency, not a correctness bug. Using>=for both would be fine.Test cases included (50 tests, 181 assertions)
Test plan
test/test_diameter_edge_cases.jl— 180 pass, 1 expected fail (asymmetric-weight bug)test/distance.jl— all 280 tests pass>=vs>condition — both correct🤖 Generated with Claude Code